Repository navigation
node:net: finish an accepted socket whose native close leaves a write parked - #43698
Conversation
… parked A send() that fails with the peer's reset consumes the socket error. The loop then sees a plain hangup, so the native close carries no error. The server-side close path failed a parked write only when the close had an error or the socket was already destroyed, because the same function also serves a half-open 'end', whose write can still drain. With neither, the socket emitted 'end' and nothing else: no write callback, no 'error', no 'close', and the server counted it until the process exited. Tell the two apart by kclosed, which only the native close handlers set. A parked write on a closed handle now fails with ERR_SOCKET_CLOSED, as it already does on the client side, and 'error' and 'close' follow.
StatusReproduced on main (release 367d939 and a debug build of a2b69f7), linux-x64, with the ledger's one-file repro: a The write takes many A deterministic form: the server blocks (stops polling) until the peer has reset, then writes. main ends with
CI on |
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughThe socket implementation now fails pending writes after closed or destroyed states, including errorless native closes. A TLS regression test verifies ordered error and close events after a peer reset during a buffered write. ChangesSocket close handling
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/net.ts— Callers that wait on a write callback still never hear back when the native close carries a read error and the socket has an 'error' listener, even after this PR. At net.ts:829-864 SocketEmitEndNT destroys the socket with the read error and returns before reaching the parked-write block at net.ts:879-882, so self[kwriteCallback] is left set and never invoked; Node delivers ECANCELED to that callback. The PR's stated goal is a JS-side guarantee for any close that leaves a write parked, but this sibling branch of the same function is excluded. Fix: fail the parked write (with the close error or ECANCELED) on every exit of SocketEmitEndNT that follows a native close, i.e. move the kwriteCallback block before the early return at net.ts:864 or into a helper both branches call.Extended reasoning...
Trigger: an accepted socket (ServerHandlers.close, net.ts:1047-1051) or an fd-connected socket (SocketHandlers.close, net.ts:611-617) closes natively with a recv error such as ECONNRESET while a write is parked in self[kwriteCallback] (set at net.ts:2998), and the user has an 'error' listener. SocketEmitEndNT takes the branch at net.ts:829, calls self.destroy(er) at net.ts:845, 859 or 862 and returns at net.ts:864. The new parked-write block at net.ts:879-882 is never reached, so the write callback is never called and self[kwriteCallback] stays set on a destroyed socket. Node's onWriteComplete calls the write callback with errnoException(UV_ECANCELED, 'write') when the handle closes with a request in flight; any code that awaits the callback (a promisified write, a queue that advances on callback) stalls forever. The base branch behaves the same, and the PR scopes it to #43250, but the PR modifies exactly this function's parked-write handling and the repository's own review rule asks that sibling paths of the same class be fixed together. Population: every server socket whose reset is…
Verification: pre-existing (the base branch fails by the exact same route; the PR does not widen it, but it does modify this function and its description mentions the gap as issue #43250 rather than closing it). Trigger: a native close that carries a read error (ECONNRESET from recv) arrives while a write is parked in
self[kwriteCallback]and the socket has an 'error' listener. Mechanism verified in…
|
On the review's extra finding (a parked write's callback is never called when the native close carries a read error and the socket has an #43250 owns that path. It takes the parked callback in The case this PR closes is the one #43250 cannot reach: the close carries no error and nothing destroys the socket, so |
The test read stderr only after stdout ended, so a child that logs more than the pipe holds would block on it and the test would hang instead of failing on the stderr assertion.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I traced the three SocketEmitEndNT callers: both native close handlers (net.ts:614, :1048) set kclosed before the call and the end handler (:655) does not, so the half-open path keeps its write parked as before, and every call that passes _err also has kclosed set, so the existing error-carrying behavior is unchanged. The sibling client-side close handler (net.ts:1548) already fails a parked write unconditionally, so this brings the server path in line rather than adding a new class of behavior.
Extended reasoning...
The change is a one-line condition swap in the tail of SocketEmitEndNT in src/js/node/net.ts (fail a parked write on kclosed || destroyed instead of destroyed || _err) plus one new subprocess test in test/js/node/tls/node-tls-server.test.ts. It touches no security-sensitive surface. A confirmed finding on the test's unbounded stdout loop is posted inline; the socket lifecycle code it modifies is fragile enough that a human look is still worthwhile, which is why this is not an approve.
Without the fix the accepted socket never emits 'close', so the test had no event to wait for and ran into its timeout. The server now reports when a second connection arrives. That handshake takes several turns of the server's loop, and the reset socket closes in the first of them or not at all. The test then fails with the recorded events and the connection count, and it checks that the child exited on its own.
There was a problem hiding this comment.
I re-reviewed the latest push and found no bugs; since this changes when node:net fails a parked write on every accepted-socket close and I could not execute the test here, a human look at the cross-platform CI result is still worthwhile.
What was reviewed:
- The
kclosedgate: it is set only in the three native close handlers, each right afterdetachSocket, so no drain can arrive afterward and a half-open'end'(viaSocketHandlers.end) never sees it set — it now matches what the client-side close handler at net.ts:1547 already does unconditionally. - The test's assertion holds whichever path observes the reset first: read-first goes through
destroy(er)and write-first through the new branch, and both yield['error', 'close:true']. - The latest commit addresses my earlier inline nit: the second connection bounds the wait and
signalCodeis asserted, so a regression now fails with the recorded events instead of timing out.
Extended reasoning...
The PR changes one condition in SocketEmitEndNT in src/js/node/net.ts so a parked write callback is failed with ERR_SOCKET_CLOSED on any native close (kclosed) rather than only when the close carries an error, and adds one subprocess test in test/js/node/tls/node-tls-server.test.ts. It touches no security-sensitive surface (no auth, crypto, or input parsing), only socket lifecycle ordering. The change is small and consistent with the sibling client-side close handler, and the author addressed the prior test-robustness nit in the last commit. Deferral rather than approval is because socket close/lifecycle in node:net has a history of platform-timing fallout across other suites, the PR's own evidence note says the test was not executed by the author locally, and I could not run a debug build or the system-Bun failure check in this environment.
|
A note for whoever reads the CI result: the evidence block in the description reads as if nobody ran the test. I ran it both ways on linux-x64 with a debug (ASAN) build.
|
2 participants
Problem
node:tlssocket whose write is the first to see the peer's RST emits'end'and nothing else: no write callback, no'error', no'close'. The server counts it forever, soserver.close()never completes. On main, 16 of 30 connections end this way. Node: 0 of 30.SocketEmitEndNT(src/js/node/net.ts:875) fails a parked write only when the native close carries an error or the socket is destroyed. Here the failedsend()consumed the socket error, so the close carries none.Fix
'end'runs the same function, and its write can still drain. Tell the two apart bykclosed, which only the native close handlers set. A parked write on a closed handle fails withERR_SOCKET_CLOSED, as the client-side close handler already does.'error'and'close'follow.test/js/node/tls/node-tls-server.test.ts(fails on main withevents: []and 2 connections, passes here).test/js/node/tls/,test/js/node/net/and Node's 325test-net-*/test-tls-*files: same failures as main.Background
net.tskeeps the callback inkwriteCallbackuntil the drain.'end'.Notes
The ledger's repro (30 connections, the peer resets on the first byte of a 1 MiB write, node peer, linux-x64)
error:ECONNRESET,close:trueerror:ECONNRESET,close:truecb:ECONNRESET,error:ECONNRESET,close:trueend,cb:ERR_SOCKET_CLOSED,error:ERR_SOCKET_CLOSED,close:trueFor the two PR rows I merged each PR's head into main @a2b69f7b. #38176 merges cleanly. #42336 conflicts in two test files only, which I resolved to main's side.
src/andpackages/merged without conflicts.Mechanism, traced with the
Socketdebug scopeThe TLS write path folds a rejected
send()to "wire blocked" and keeps no errno (that is #42336). A plainnet.createServerdoes not reach this state, becauseus_socket_write_check_errorreports the errno andfailWritefails the write.What still differs from Node after this change
The sockets that took this path report
'end', thenERR_SOCKET_CLOSED. Node reportswrite ECONNRESETand no'end'. The errno is gone by the time the close reaches JS, so only the native fix can restore it. With #42336 the write fails at write time and nothing is parked at the close, so the two changes do not overlap. If #42336 lands first, the new test here passes without this change, and this becomes a guard with no known trigger on Linux.The test
The server runs in a child process so that it can stop polling. It reports the accepted socket, then blocks in
fs.readSync(0)until the test has reset the connection, and only then writes. The write is therefore the first operation to see the reset on every run, with no timing involved:A socket that never closes gives no event to wait for, so the server reports when a second connection arrives. That handshake takes several turns of the server's loop, and the reset socket closes in the first of them or not at all. The server then destroys both sockets, so the child exits in both outcomes and the test fails with the recorded state, not with a timeout (about 2 s on a debug build of main).
The test asserts what holds in Node and on every platform:
'error', then'close'withhadErrortrue, then agetConnections()of 1, which is the second connection. It also asserts that the child exited on its own. It does not pin the error code or the write callback. On main the read-error path never calls a parked write's callback, which is #43250. If the reset were seen by a read first, a fixed build would still pass.Overlap with open PRs
#43392 rewrites the same block into a helper and keeps the same
destroyed || _errcondition, so it does not cover this case. The two conflict textually in that one hunk. The resolution is to keep its helper and call it whenkclosedis set.no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/tls/node-tls-server.test.ts